🤖 Codex PR Review

🚫 Merge blocked: 2 serious issue(s) found in high-risk files

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in scripts/emit_pr_review_event.py

The deduplication key only includes repository, pr_number, head_sha, and GITHUB_RUN_ID. GitHub reruns keep the same run_id and only increment GITHUB_RUN_ATTEMPT, so a rerun with a different final outcome will be treated as an already-sent duplicate and its notification will be dropped. That can leave operators with a stale failure/success alert that no longer matches the latest workflow attempt. (line 101)

Suggestion: Include GITHUB_RUN_ATTEMPT in the emitted metadata and in run_id, and validate/persist it on the service side so each rerun attempt is tracked independently.

2. 🟠 [HIGH] Logic in .github/workflows/codex_pr_review.yml

The new event step runs under if: always(), and the emitter accepts skipped as a valid review outcome. If an earlier guard/setup step fails before Run Codex PR Review executes at all (for example fork rejection, bridge checkout failure, or Python setup failure), the workflow can still emit a pr_review_completed event with review_outcome=skipped, producing a false completion notification for a review that never happened. (line 158)

Suggestion: Only emit the event when the review step actually ran, or reject skipped for this event type. A simple fix is to gate the step on a non-empty/non-skipped steps.review.outcome or require the review decision artifact to exist before emitting.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Reliability in scripts/emit_pr_review_event.py

The emitter treats any notification status other than sent or deduplicated as a hard delivery failure, but the service intentionally returns skipped when telegram.env is not configured. Because the PR explicitly makes that environment file optional, every review run against an unprovisioned service will retry three times and end with a warning even though the route handled the event as designed. (line 175)

Suggestion: Treat notification.status == "skipped" as a successful no-op when Telegram is intentionally unconfigured, or suppress emission earlier when notification delivery is disabled.

2. 🟡 [MEDIUM] Reliability in service/review_event_notification.py

dispatch_review_event_notification() records only a single boolean result from send_telegram_alert(), but that helper sends to multiple chat IDs. If one chat succeeds and another fails, the whole event is recorded as failed and will be retried, causing duplicate messages to the chats that already received it. That breaks the intended one-event notification semantics whenever multiple chat IDs are configured. (line 140)

Suggestion: Return per-chat delivery results instead of a single boolean, and persist which recipients have already been delivered so retries only target the remaining failures.

Review by Codex PR Review bot • PR

PERMANENT_FREEZE/CLOSE_RESLICE: final closure is exhausted. Current-head review found two reachable blockers: rerun attempts share the same notification identity, and a skipped review step can still emit a completion event. Evidence, branch, commits, tests, and review history are preserved. No second closure, rerun, merge, deploy, or automatic successor.